Skip to content

Monetizable: Skip subunit write for non-attributes in getter #706

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Aug 27, 2025
Merged

Monetizable: Skip subunit write for non-attributes in getter #706

merged 4 commits into from
Aug 27, 2025

Conversation

edithemmings
Copy link
Contributor

@edithemmings edithemmings commented Oct 11, 2024 •
edited
Loading

Discovered an issue where making an update to instance currencies causes a MissingAttributeError for converted methods

Example model:

class Transaction < ActiveRecord::Base
  monetize :amount_cents, with_model_currency: :currency
  monetize :tax_cents, with_model_currency: :currency
  monetize :total_cents, with_model_currency: :currency

  def total_cents
    amount_cents + tax_cents
  end
end

Triggering the error:

transaction = Transaction.create(currency: 'USD')
transaction.update(currency: 'CAD')
ActiveModel::MissingAttributeError: can't write unknown attribute `total_cents`

This occurs because of a condition in read_monetized that tries to update the attribute if the currency changes. This case is needed for maintaining persisted values if the currency changes in a way that the subunit to unit ratio also changes. (This logic is also referred to as being controversial in the TODO comment on the test coverage)

Regardless, the attempt to update the attribute fails for monetized methods because there is no attribute to update. So I added a condition to check for the attribute's presence.

@edithemmings edithemmings force-pushed the main branch 2 times, most recently from 2ca99c0 to fbc4f21 Compare October 12, 2024 15:52
@edithemmings edithemmings changed the title Monetizable: Ignore converted methods when writing attributes Monetizable: Skip subunit write for non-attributes in getter Oct 12, 2024
sunny
sunny approved these changes Jul 9, 2025
View reviewed changes
Copy link
Contributor

@sunny sunny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Could you add a small entry to the Changelog as well? 🙏🏻

@edithemmings edithemmings requested a review from sunny August 12, 2025 19:21
@wwahammy
Copy link

wwahammy commented Aug 27, 2025 •
edited
Loading

@yukideluxe, hope you don't mind me @'ing you directly. First off, thanks for all you do on Money Rails. It's a wonderful tool.

Is there anything we can to help move this PR to be ready for the Money Rails team review? The work you all do is truly valuable so we want to make sure we're not wasting your time. Also, is there anything generally that we can do to help the team be more effective?

@yukideluxe
Copy link
Member

@yukideluxe, hope you don't mind me @'ing you directly. First off, thanks for all you do on Money Rails. It's a wonderful tool.

Is there anything we can to help move this PR to be ready for the Money Rails team review? The work you all do is truly valuable so we want to make sure we're not wasting your time. Also, is there anything generally that we can do to help the team be more effective?

Hi @wwahammy! You can ping me anytime! Sorry, it's been a crazy August 😳 I will review and merge the PR ASAP! 💪🏻

yukideluxe
yukideluxe approved these changes Aug 27, 2025
View reviewed changes
Copy link
Member

@yukideluxe yukideluxe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Thanks for the fix!

@wwahammy
Copy link

Hi @wwahammy! You can ping me anytime! Sorry, it's been a crazy August 😳 I will review and merge the PR ASAP! 💪🏻

Nothing to apologize for at all, @yukideluxe. The work you do is awesome and we're all incredibly thankful. If there's anything any of us can do to chip in and help, let us know.

@yukideluxe yukideluxe merged commit 79df005 into RubyMoney:main Aug 27, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunny sunny sunny approved these changes

@yukideluxe yukideluxe yukideluxe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@edithemmings @wwahammy @yukideluxe @sunny